Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] feat: help users use rclip with image viewers on Windows #141

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheChaoticor
Copy link

solved,connected with xnview

solved,connected with xnview
@TheChaoticor
Copy link
Author

Please review it

@@ -216,7 +219,7 @@ def main():
args.exclude_dir,
args.no_indexing,
)

XNVIEW_PATH = "C:\\Program Files\\XnViewMP\\xnviewmp.exe"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What if I have xnview installed to a different path?
  2. What if I don't have xnview installed?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why xnview? What about other image viewers.

@@ -228,6 +231,13 @@ def main():
print(f'{r.score:.3f}\t"{r.filepath}"')
if args.preview:
preview(r.filepath, args.preview_height)
if os.path.exists(XNVIEW_PATH):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this always try to open the xnview?

  1. What if I am on Linux/macOS?
  2. What if I don't want to open xnview?

Copy link
Owner

@yurijmikhalevich yurijmikhalevich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the initiative 🙌

But the design of this PR is bad:

  1. We probably shouldn't require the user to have xnview installed to use previews on Windows.
  2. We shouldn't hardcode the path to xnview.
  3. We shouldn't open result images in xnview by default.

A better version of this PR could be:

  • no changes to rclip code
  • a change to README.md showing how to use xnview together with rclip to view files

Another option may involve building a generic adapter/helper that may try calling an image viewer if the user passed a CLI arg to ask for this, but this is more involved, and I am not sure if we need to do it now. But happy to discuss different approach ideas.

Can you change this PR to:

  • remove changes to the rclip code
  • add an example on how to use rclip with xnview, similar to this one:

    rclip/README.md

    Lines 121 to 137 in 01801ea

    <details>
    <summary>Using a different terminal or viewer</summary>
    If you are using any other terminal or want to view the results in your viewer of choice, you can pass the output of **rclip** to it. For example, on Linux, the command from below will open top-5 results for "kitty" in your default image viewer:
    ```bash
    rclip -f -t 5 kitty | xargs -d '\n' -n 1 xdg-open
    ```
    The `-f` param or `--filepath-only` makes **rclip** print the file paths only, without scores or the header, which makes it ideal to use together with a custom viewer as in the example.
    I prefer to use **feh**'s thumbnail mode to preview multiple results:
    ```bash
    rclip -f -t 5 kitty | feh -f - -t
    ```
    </details>

Thank you, and let me know if you have any questions 😄

@TheChaoticor
Copy link
Author

Thank you for your review.I was inspired by the issue and suggestion on xnview and i misunderstood thinking that the pr can be made successful by just connecting with xnview .I will improve my pr and let you know

@yurijmikhalevich yurijmikhalevich changed the title #82 [WIP] feat: help users use rclip with image viewers on Windows Oct 12, 2024
@yurijmikhalevich yurijmikhalevich marked this pull request as draft October 12, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants